Dereference one less on search_is_some and make it auto-fixable #4454

ghost · 2019-08-26T10:51:51Z

Fixes #4453

changelog: none

ghost · 2019-08-26T10:54:12Z

@flip1995 do you think this is enough, or is some logic required?
If so, could you mentor me?

I can make it auto fixable once everything else works 😉

How can I run doc tests? cargo test --doc SEARCH_IS_SOME doesn't work for me :/

phansch · 2019-08-26T11:03:56Z

How can I run doc tests? cargo test --doc SEARCH_IS_SOME doesn't work for me :/

I think you'll first have to change to the clippy_lints directory for them to work.

clippy_lints/src/methods/mod.rs

ghost · 2019-08-26T13:47:24Z

I though this would be easier and the code better to understand. But it seems that I am not capable of fixing the issue 😢

flip1995 · 2019-08-26T14:23:58Z

Your not really far from the goal, and I'm happy to support you to achieve it!

You already have the closure argument: closure_arg
You only need to care about closure arguments, that are PatKind::Lit(expr)
Get the closure_arg_snip = snippet(cx, expr, "..") (This is the name of the closure argument)
Run search_snippet.replace(&format!("*{}", closure_arg_snip), &closure_arg_snip)

For test cases, see Playground

ghost · 2019-08-26T16:01:08Z

I think you'll first have to change to the clippy_lints directory for them to work.

Thanks. That worked 😃

ghost · 2019-08-26T16:02:14Z

@flip1995 thank you for the help 👍

Are you sure I need closure_arg and not the closure_body.value?
PatKind::Lit never finds a match

flip1995 · 2019-08-26T16:41:51Z

Huh, I'm pretty sure that you need closure_arg. body.value is the actual body of the closure.

It should find a match for _.find(|x| ..), except something else is meant by PatKind::Lit. What you can do to find out the correct PatKind for closures like this, is to add a dbg!(closure_arg.node) to the code and then look at the stderr output, when running the testcase: TESTNAME=ui/<name_of_search_is_some_test_without_.rs> cargo +master uitest

ghost · 2019-08-26T19:34:16Z

LL |     let _ = (0..1).find(|x| *x == 0).is_some();
   |             ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ help: try this: `any(|x| x == 0)`

it seems like the lint isn't very exact. This causes rust-fix to replace everything with just any(...)

let _ = any(|x| x == 0);

Or did I do something wrong?

flip1995 · 2019-08-26T19:57:33Z

Rustfix always replaces the span you specify with the suggested code. You have to build the span by hand, since it does not exist in the way you need it. This shouldn't be a problem though. Just search in the methods/mod.rs file for span.with_hi to see how to do this.

ghost · 2019-08-27T17:59:03Z

Rustfix always replaces the span you specify with the suggested code

so I should be able to replace expr.span, with a formatted {0}({1}).is_some(), right?

flip1995 · 2019-08-28T08:26:54Z

No, because the expr.span is everything from the iterator expression ((0..1). in the example above, or e.g. vec.iter().) up to the call of is_some(). Or in other words, everything that is indicated by the ^ in the error message.

You only want to replace the find(..).is_some() with any(..) though. That means you have to construct a span, that starts at find... and ends at ...some(). You already have a span that ends at ...some(): expr.span. Now you need a span that starts with find....

I just looked up how to get this span, and saw, that it is not that easy (and not really easy to explain the changes that need to be made). So I decided to write a patch for you, that simplifies this. (I'll notify you once it's ready)

While looking into this, I found, that there already exists a function to get the name of the closure arg:

rust-clippy/clippy_lints/src/utils/mod.rs

Lines 916 to 922 in 949b347

    
           pub fn get_arg_name(pat: &Pat) -> Option<ast::Name> { 
        
               match pat.node { 
        
                   PatKind::Binding(.., ident, None) => Some(ident.name), 
        
                   PatKind::Ref(ref subpat, _) => get_arg_name(subpat), 
        
                   _ => None, 
        
               } 
        
           }

You can just use this.

flip1995 · 2019-08-28T08:44:07Z

Here's the patch: https://github.com/flip1995/rust-clippy/commits/method_calls_spans

You can look at the second to last commit on the branch on how to use the method spans. Just git cherry-pick the commits on this PR.

bors · 2019-08-28T10:58:37Z

☔ The latest upstream changes (presumably #4462) made this pull request unmergeable. Please resolve the merge conflicts.

ghost · 2019-08-29T08:08:05Z

almost worked

clippy_lints/src/methods/mod.rs

bors · 2019-08-29T11:30:09Z

☔ The latest upstream changes (presumably #4408) made this pull request unmergeable. Please resolve the merge conflicts.

phansch · 2019-08-29T15:27:42Z

Sorry about the merge conflict, let me know if I can help resolving it =)

flip1995 · 2019-08-29T15:29:39Z

That was my fault in 6035b65, so I'd also resolve it for you, if need be.

ghost · 2019-08-29T15:31:08Z

I don't get what is going on.
Rebasing just broke my lint again

Please fix it for me :)

flip1995 · 2019-08-29T15:36:01Z

Done 👍

tests/ui/methods.stderr

clippy_lints/src/methods/mod.rs

flip1995 · 2019-09-02T11:02:41Z

Could you add a test case where x is derefed multiple times:

let _ = vec.iter().find(|x| **x == 0).is_some();

clippy_lints/src/methods/mod.rs

flip1995 · 2019-09-03T14:32:23Z

Ok one last change and a run of ./util/dev fmt and this is ready to merge. Sorry that this was harder, than expected. I'm really bad when it comes to judging if an issue is a good first issue 😄

flip1995 · 2019-09-04T07:53:41Z

Thanks!

@bors r+

bors · 2019-09-04T07:53:42Z

📌 Commit 64cd9e4 has been approved by flip1995

bors · 2019-09-04T07:53:49Z

⌛ Testing commit 64cd9e4 with merge 8239b76...

Dereference one less on search_is_some and make it auto-fixable Fixes #4453 changelog: none

bors · 2019-09-04T08:21:50Z

☀️ Test successful - checks-travis, status-appveyor
Approved by: flip1995
Pushing 8239b76 to master...

Previously the program only fixed the code when the closure supplied to the function contained only 1 line. This patch removes the restriction. The code already works. This patch only removes the extra check that causes the restriction. The test cases that can now be fixed are moved into the files containing tests cases that can be fixed. The extra check has survived in the code this way. - In Dec 2015, patch a6bd2d0, pull request rust-lang#524. The lint was first added. The program did not support fixing code automatically yet. So the suggested fix was printed as a part of the diagnostic message. When the original code contained multiple lines, the suggested fix was omitted in order to keep the diagnostic message concise. - In May 2019, patch bd0b75f, pull request rust-lang#4049. Logic was added to strip the reference in the closure when the suggested replacement method required it. Because the fix was still only printed when the code contained a single line, the new transformation was only done when the code contained a single line. - In Aug 2019, patch 945d4cf, pull request rust-lang#4454. The lint was updated to fix code automatically. Because the fixed code had only been printed in the diagnostic message for a single line, the fix was only added for a single line. - In Nov 2021, patch 092fe20, pull request rust-lang#7463. The logic for transforming the closure was moved into another file. A comment was added saying that it was only good for a single line because it had only been used for a single line. changelog: [`search_is_some`] now fixes code spanning multiple lines

Previously the program only fixed the code when the closure supplied to the method contained only 1 line. This patch removes the restriction. The code already works. This patch only removes the extra check that causes the restriction. The test cases that can now be fixed are moved into the files containing tests cases that can be fixed. The extra check has survived in the code this way. - In Dec 2015, patch a6bd2d0, pull request rust-lang#524. The lint was first added. The program did not support fixing code automatically yet. So the suggested fix was printed as a part of the diagnostic message. When the original code contained multiple lines, the suggested fix was omitted in order to keep the diagnostic message concise. - In May 2019, patch bd0b75f, pull request rust-lang#4049. Logic was added to strip the reference in the closure when the suggested replacement method required it. Because the fix was still only printed when the code contained a single line, the new transformation was only done when the code contained a single line. - In Aug 2019, patch 945d4cf, pull request rust-lang#4454. The lint was updated to fix code automatically. Because the fixed code had only been printed in the diagnostic message for a single line, the fix was only added for a single line. - In Nov 2021, patch 092fe20, pull request rust-lang#7463. The logic for transforming the closure was moved into another file. A comment was added saying that it was only good for a single line because it had only been used for a single line. changelog: [`search_is_some`] now fixes code spanning multiple lines

Previously the program only fixed the code when the closure supplied to the method contained only 1 line. This patch removes the restriction. The code already works. This patch only removes the extra check that causes the restriction. The test cases that can now be fixed are moved into the files containing tests cases that can be fixed. The unnecessary check has survived in the code this way. - In Dec 2015, patch a6bd2d0, pull request rust-lang#524. The lint was first added. The program did not support fixing code automatically yet. So the suggested fix was printed as a part of the diagnostic message. When the original code contained multiple lines, the suggested fix was omitted in order to keep the diagnostic message concise. - In May 2019, patch bd0b75f, pull request rust-lang#4049. Logic was added to strip the reference in the closure when the suggested replacement method required it. Because the fix was still only printed when the code contained a single line, the new transformation was only done when the code contained a single line. - In Aug 2019, patch 945d4cf, pull request rust-lang#4454. The lint was updated to fix code automatically. Because the fixed code had only been printed in the diagnostic message for a single line, the fix was only added for a single line. - In Nov 2021, patch 092fe20, pull request rust-lang#7463. The logic for transforming the closure was moved into another file. A comment was added saying that it was only good for a single line because it had only been used for a single line. changelog: [`search_is_some`] now fixes code spanning multiple lines

flip1995 requested changes Aug 26, 2019

View reviewed changes

clippy_lints/src/methods/mod.rs Show resolved Hide resolved

flip1995 added the S-waiting-on-author Status: This is awaiting some action from the author. (Use `@rustbot ready` to update this status) label Aug 26, 2019

ghost changed the title ~~WIP: Dereference one less on search_is_some~~ Dereference one less on search_is_some and make it auto-fixable Aug 26, 2019

flip1995 reviewed Aug 29, 2019

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

BO41 and others added 4 commits August 29, 2019 17:34

Dereference one less on search_is_some and make it auto-fixable

945d4cf

Also return the method spans in utils::method_calls

832c083

Use the spans returned by utils::method_calls

dac81d8

Fix tests

6bbf418

ghost commented Aug 30, 2019

View reviewed changes

tests/ui/methods.stderr Outdated Show resolved Hide resolved

flip1995 reviewed Sep 2, 2019

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

flip1995 reviewed Sep 3, 2019

View reviewed changes

clippy_lints/src/methods/mod.rs Outdated Show resolved Hide resolved

Try to fix .fixed

64cd9e4

flip1995 approved these changes Sep 4, 2019

View reviewed changes

bors added a commit that referenced this pull request Sep 4, 2019

Auto merge of #4454 - BO41:search_is_some, r=flip1995

8239b76

Dereference one less on search_is_some and make it auto-fixable Fixes #4453 changelog: none

bors merged commit 64cd9e4 into rust-lang:master Sep 4, 2019

ghost deleted the search_is_some branch September 4, 2019 16:57

qmwrygdxpzc mentioned this pull request Oct 16, 2025

feat(search_is_some): Fix when the closure spans multiple lines #15902

Open

Dereference one less on search_is_some and make it auto-fixable #4454

Dereference one less on search_is_some and make it auto-fixable #4454

Uh oh!

Conversation

ghost commented Aug 26, 2019

Uh oh!

ghost commented Aug 26, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

phansch commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

ghost commented Aug 26, 2019

Uh oh!

flip1995 commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 26, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 26, 2019 • edited by ghost Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flip1995 commented Aug 26, 2019

Uh oh!

ghost commented Aug 26, 2019

Uh oh!

flip1995 commented Aug 26, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Aug 27, 2019

Uh oh!

flip1995 commented Aug 28, 2019

Uh oh!

flip1995 commented Aug 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

bors commented Aug 28, 2019

Uh oh!

ghost commented Aug 29, 2019

Uh oh!

Uh oh!

bors commented Aug 29, 2019

Uh oh!

phansch commented Aug 29, 2019

Uh oh!

flip1995 commented Aug 29, 2019

Uh oh!

ghost commented Aug 29, 2019

Uh oh!

flip1995 commented Aug 29, 2019

Uh oh!

Uh oh!

Uh oh!

flip1995 commented Sep 2, 2019

Uh oh!

Uh oh!

flip1995 commented Sep 3, 2019

Uh oh!

flip1995 commented Sep 4, 2019

Uh oh!

bors commented Sep 4, 2019

Uh oh!

bors commented Sep 4, 2019

Uh oh!

bors commented Sep 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

ghost commented Aug 26, 2019 •

edited by ghost

Loading

phansch commented Aug 26, 2019 •

edited

Loading

flip1995 commented Aug 26, 2019 •

edited

Loading

ghost commented Aug 26, 2019 •

edited by ghost

Loading

ghost commented Aug 26, 2019 •

edited by ghost

Loading

flip1995 commented Aug 26, 2019 •

edited

Loading

flip1995 commented Aug 28, 2019 •

edited

Loading